Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

gh-358: Add mypy CI and config #518

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

erlend-aasland
Copy link
Contributor

Estalish a minimal mypy config that ensures annotations are compatible
with Python 3.8 (the oldest supported Python version).

Add a forgiving CI job that does not affect the status of the GitHub
workflow run.

Estalish a minimal mypy config that ensures annotations are compatible
with Python 3.8 (the oldest supported Python version).

Add a forgiving CI job that does not affect the status of the GitHub
workflow run.
@codecov-commenter
Copy link

codecov-commenter commented Jul 11, 2024

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 71.22%. Comparing base (6bc90a8) to head (5b8d3e9).

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #518      +/-   ##
==========================================
+ Coverage   71.16%   71.22%   +0.06%     
==========================================
  Files          26       26              
  Lines        3114     3114              
  Branches      527      480      -47     
==========================================
+ Hits         2216     2218       +2     
+ Misses        766      765       -1     
+ Partials      132      131       -1     

see 2 files with indirect coverage changes

@erlend-aasland
Copy link
Contributor Author

erlend-aasland commented Jul 11, 2024

This shows up red in the PR list:

Screenshot 2024-07-11 at 13 00 33

... and green in the Actions log:

Screenshot 2024-07-11 at 13 00 15

The former might be mitigated (and thus made green) by flipping some project configuration settings and making it a non-required check; I'm not quite sure where that setting lives, though.

@acolomb
Copy link
Collaborator

acolomb commented Aug 6, 2024

@christiansandberg seems like only you have the necessary access privileges to configure repository settings. I can look into the matter if you give me the permissions.

@christiansandberg
Copy link
Owner

Do you have some instructions on how to configure that? It also seems I can't change the level of access contributors have in GitHub either.

'pyproject.toml'
'requirements-dev.txt'
- run: pip install -r requirements-dev.txt -e .
- run: mypy --config-file pyproject.toml .
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe just silence the return code?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would work as a last resort, but I'd like to actually see the status already while we are in the process of making it pass.

@acolomb
Copy link
Collaborator

acolomb commented Aug 7, 2024

The settings for checks should be somewhere under branch protection for the master branch. Sorry, I don't have any repository at hand where I could search the setting myself. None where I am the owner and checks are defined...

The level of access needed would be to change repository settings, but I can't tell how that would be assigned.

@christiansandberg
Copy link
Owner

Can't see that it is possible to sort of "white-list" a specific step so that the whole action becomes green.

It might also be hard for a new contributor to know what to do about the failed step since they can't fix it.

@erlend-aasland
Copy link
Contributor Author

Can't see that it is possible to sort of "white-list" a specific step so that the whole action becomes green.

I suspect the mypy CI would have to live in its own YAML file in order to be selectable.

@acolomb
Copy link
Collaborator

acolomb commented Aug 9, 2024

I've asked the folks at Syncthing, where I've seen such a setup: https://forum.syncthing.net/t/ot-question-about-github-checks/22577

@acolomb
Copy link
Collaborator

acolomb commented Aug 9, 2024

@christiansandberg does this help? https://forum.syncthing.net/t/ot-question-about-github-checks/22577/2?u=acolomb

It might not show the new check yet, as it is not merged to master. We could set up a separate branch to test it though.

@christiansandberg
Copy link
Owner

Yes, it should show up once this is merged to master I hope.

image

@christiansandberg
Copy link
Owner

But I guess it won't be any different compared to now since status checks do not block merging? I think it still may cause confusion if you don't know that some checks are OK to fail.

@acolomb
Copy link
Collaborator

acolomb commented Aug 9, 2024

IIRC, the pull request will be marked with a green checkmark although such an optional check is failing. In the list of checks, all but the mypy check will get a "Required" label attached.

This all depends on actually enabling the branch protection of course, to disallow merging with failing checks at all. Which is a good idea in general. The second step is then to exempt the specific mypy check as "non-required".

Once we actually have fixed it to not fail, we can remove that exemption again - or leave it active as I think typing checks are still somewhat icing on the cake and could be handled separately after merging a feature PR.

If all this cannot be accomplished with the GitHub settings, then we might just as well concentrate on fixing the actual errors before enabling the CI check. But let's try getting it to work first 😉

@erlend-aasland
Copy link
Contributor Author

Yes, it should show up once this is merged to master I hope.

Yes, I think you're right that it needs to be on a protected branch, such as master, in order for it to be selectable. From your screen-shot, it seems to me it does not have to be defined in a separate YAML file, since you already get to choose from the feature matrix in the CI.

Anyway, let's hold this PR until CI is green again (see commit d1c28e5).

@erlend-aasland
Copy link
Contributor Author

[...] We could set up a separate branch to test it though.

That's a very good idea.

@acolomb
Copy link
Collaborator

acolomb commented Aug 9, 2024

I've created https://github.com/christiansandberg/canopen/tree/mypy with this PR merged into it. It doesn't run the actions at all though, probably CI is only enabled for master. Could use that as a playground to fiddle with the settings though. If you're so inclined @christiansandberg 😉

@erlend-aasland
Copy link
Contributor Author

I've created https://github.com/christiansandberg/canopen/tree/mypy with this PR merged into it. It doesn't run the actions at all though, probably CI is only enabled for master. Could use that as a playground to fiddle with the settings though.

Nice, thanks! I've created a test PR against it:

@acolomb
Copy link
Collaborator

acolomb commented Aug 11, 2024

@christiansandberg it still would help if you could give me access to the repository settings, so I can adjust the protection rules directly. At least until we have found the right configuration.

@erlend-aasland
Copy link
Contributor Author

@christiansandberg it still would help if you could give me access to the repository settings, so I can adjust the protection rules directly. At least until we have found the right configuration.

Gentle ping, @christiansandberg 🏓

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants